Multi circuit proof integration #1510
Conversation
synthesizer/src/process/mod.rs
Outdated
| let mut all_assignments = Vec::with_capacity(1); | ||
| let (_response, mut execution, _inclusion, _metrics, function_assignments) = | ||
| process.prepare_function::<CurrentAleo, _>(authorization, rng).unwrap(); | ||
| let function_assignments = Arc::try_unwrap(function_assignments).unwrap_or_default().into_inner(); // TODO: return Err if there is one |
There was a problem hiding this comment.
IIRC an existence of Arc clones here would indicate a leak/logic error
There was a problem hiding this comment.
Yes in a next iteration I'm following the same approach we use for e.g. execution/inclusion, just try_unwrap
synthesizer/src/snark/batch/bytes.rs
Outdated
| // TODO: instead of unwrap, convert the error to an IoError | ||
| let batch_len = usize::try_from(u32::read_le(&mut reader)?).unwrap(); |
There was a problem hiding this comment.
is there a plan to support any 16bit architectures? if not, casting u32 to usize should never fail
There was a problem hiding this comment.
I think with that assumption in hand this PR would also be a lot simpler: https://github.com/AleoHQ/snarkVM/pull/1499
But we spent so much time on it already that I think not assuming that is fine for now.
vicsn
force-pushed
the
multi-circuit-proof-integration
branch
2 times, most recently
from
bc49de5 to
24a7dca
Compare
vicsn
force-pushed
the
multi-circuit-proof-integration
branch
from
d9cb92d to
df58d23
Compare
| impl<N: Network> Ord for ProvingKeyId<N> { | ||
| /// Ordering is determined by the program_id first, then the function_name second. | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| match self.program_id == other.program_id { | ||
| true => self.function_name.cmp(&other.function_name), | ||
| false => self.program_id.cmp(&other.program_id), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<N: Network> PartialOrd for ProvingKeyId<N> { | ||
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } |
There was a problem hiding this comment.
this should be identical to an automatic #[derive(PartialOrd, Ord)]: the program_id is compared first, and if it's the same, the function_name is compared (source).
There was a problem hiding this comment.
Due to the way the derived Ord/PartialOrd works, the compiler also demands the networktype (Testnet3) to have those derivations, and to adjust a large amount of trait bounds to be updated to N: Network+Ord+PartialOrd. I think keep the code as is would be better.
For reference, a non-compiling partial implementation:
diff --git a/console/network/src/testnet3.rs b/console/network/src/testnet3.rs
index b29dcf99e..2ebd3435d 100644
--- a/console/network/src/testnet3.rs
+++ b/console/network/src/testnet3.rs
@@ -75,7 +75,7 @@ lazy_static! {
};
}
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, Ord, PartialOrd)]
pub struct Testnet3;
impl Testnet3 {
diff --git a/synthesizer/src/block/transition/mod.rs b/synthesizer/src/block/transition/mod.rs
index 1ac051851..863409eb9 100644
--- a/synthesizer/src/block/transition/mod.rs
+++ b/synthesizer/src/block/transition/mod.rs
@@ -431,24 +431,8 @@ impl<N: Network> Transition<N> {
}
}
-#[derive(Clone, Copy, PartialEq, Eq)]
-pub struct ProvingKeyId<N: Network> {
+#[derive(Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
+pub struct ProvingKeyId<N: Network + Ord> {
pub program_id: ProgramID<N>,
pub function_name: Identifier<N>,
-}
-
-impl<N: Network> Ord for ProvingKeyId<N> {
- /// Ordering is determined by the program_id first, then the function_name second.
- fn cmp(&self, other: &Self) -> Ordering {
- match self.program_id == other.program_id {
- true => self.function_name.cmp(&other.function_name),
- false => self.program_id.cmp(&other.program_id),
- }
- }
-}
-
-impl<N: Network> PartialOrd for ProvingKeyId<N> {
- fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
- Some(self.cmp(other))
- }
-}
+}
\ No newline at end of file
diff --git a/synthesizer/src/process/execute.rs b/synthesizer/src/process/execute.rs
index eebdd5249..d761acc06 100644
--- a/synthesizer/src/process/execute.rs
+++ b/synthesizer/src/process/execute.rs
@@ -18,7 +18,7 @@ use super::*;
use crate::{Key, KeyBatch, KeyMode, ProvingKeyId};
-impl<N: Network> Process<N> {
+impl<N: Network + Ord + PartialOrd> Process<N> {
| @@ -66,7 +67,6 @@ impl<N: Network> TransitionStorage<N> for TransitionMemory<N> { | |||
| input_store: InputStore::open(dev)?, | |||
| output_store: OutputStore::open(dev)?, | |||
| finalize_map: MemoryMap::default(), | |||
| proof_map: MemoryMap::default(), | |||
There was a problem hiding this comment.
should we mark the corresponding DataID as deprecated? if we don't intend to use it anymore, we can remove it before the next network reset
There was a problem hiding this comment.
related: https://github.com/AleoHQ/snarkVM/pull/1560; also, only the DataID variant needs to remain, so as to not perturb the order (and values) of the others (unless we explicitly assign values to the DataID variants)
There was a problem hiding this comment.
On advice from Howard, we're merging this PR into staging, allowing us to just remove the DataIDs
vicsn
force-pushed
the
multi-circuit-proof-integration
branch
from
cd0a0b1 to
a5fb2b7
Compare
vicsn
force-pushed
the
multi-circuit-proof-integration
branch
from
50ab2d3 to
71d7a63
Compare
clean up access of last element
vicsn
force-pushed
the
multi-circuit-proof-integration
branch
from
71d7a63 to
7f4a4c4
Compare
Motivation
This adds support for batch proving executions and their inclusions.
This is a very rough first draft. I suggest we do the review in two parts: first only important architecture comments on e.g. the callchain, function signatures and objects. There is a lot of repetition in the code, and existing TODOs of mine, so please save yourself time by mentioning your ideas only once.
Some notes on this PR:
verify_execution<const VERIFY_INCLUSION: bool>which was never actually used, becauseverify_executionwould still check whether the execution had an inclusion proof.vm.execute()andvm.execute_fee()to avoid us having tocast_ref!parameters up and down the call chain.Executemode of theCallstackby aPreparemode - running execute_function(...) in Prepare mode doesn't create a proof so instead the caller can afterwards extract the filled registers.inherent associated types are unstableif we try sth like: type ExecutionAssignments<'a, N> = Vec<&Assignment<N::Field>>;Test Plan
sample_execution_transaction_without_feecalls transfer() but fails as expected before verifying the inclusionsample_execution_transaction_with_feecalls mint() which doesn't require an input record and correspondinginclusion proof. I adjusted the test now to call transfer(), there are still other tests which call mint().
Related PRs and design document
https://github.com/AleoHQ/snarkVM/pull/1382
https://docs.google.com/document/d/10f_VfrcL9POyBPdFWVov5vVieVL-YNW6gBCxidEoNq8/edit#heading=h.9e2ahww0jjqr